Allow JavaScript binding when browser wrapper cannot be found#5229
Allow JavaScript binding when browser wrapper cannot be found#5229amaitland merged 3 commits intocefsharp:masterfrom
Conversation
📝 WalkthroughWalkthroughMoved per-browser JavaScript binding configuration out of Changes
Sequence Diagram(s)sequenceDiagram
participant Browser as Browser (CefBrowser)
participant Subproc as CefAppUnmanagedWrapper
participant Store as _browserJavascriptBindingSettings
participant Frame as Frame (CefFrame)
Browser->>Subproc: OnBrowserCreated(browser)
Subproc->>Store: create & store JavascriptBindingSettings[browserId]
Subproc->>Subproc: _browserWrappers->TryAdd(browserId, wrapper)
Frame->>Subproc: OnContextCreated(frame, browserId)
Subproc->>Store: lookup JavascriptBindingSettings[browserId]
Subproc->>Subproc: IsJavascriptBindingApiAllowed(settings, frame)
alt allowed
Subproc->>Frame: inject CefSharp JS binding
else not allowed
Subproc->>Frame: skip injection
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CefSharp.BrowserSubprocess.Core/CefAppUnmanagedWrapper.cpp`:
- Around line 352-363: IsJavascriptBindingApiAllowed currently returns true when
browserWrapper is null, bypassing per-browser enforcement; change the
null-wrapper path in IsJavascriptBindingApiAllowed to treat a missing
browserWrapper as not allowed (return false) so that JavascriptBindingApiEnabled
and JavascriptBindingApiHasAllowOrigins checks are enforced, and add unit tests
that simulate a missing browserWrapper where (1) JavascriptBindingApiEnabled is
false and (2) JavascriptBindingApiHasAllowOrigins is true to verify bindings are
not injected when the wrapper lookup fails.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2b1d8844-dad5-4b5d-ab77-aff326ee7e6b
📒 Files selected for processing (1)
CefSharp.BrowserSubprocess.Core/CefAppUnmanagedWrapper.cpp
|
❌ Build CefSharp 146.0.70-CI5456 failed (commit caa26a55be by @campersau) |
amaitland
left a comment
amaitland
left a comment
There was a problem hiding this comment.
Thanks for the PR.
As a short term workaround this is probably ok, though I'd like to resolve this in a different way.
This avoids the issue that sometimes browsers are destroyed but then referenced again without them beeing created
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CefSharp.BrowserSubprocess.Core/CefAppUnmanagedWrapper.h`:
- Around line 76-83: The cleanup loop is iterating the ConcurrentDictionary
directly with OfType<JavascriptBindingSettings^> which yields nothing; update
the block that checks _browserJavascriptBindingSettings to iterate its values
(e.g., use _browserJavascriptBindingSettings->Values or iterate KeyValuePair and
use kvp.Value) and delete each JavascriptBindingSettings instance
(JavascriptBindingSettings^) before setting _browserJavascriptBindingSettings =
nullptr; keep the existing null check and ensure you delete the value objects
rather than the pairs.
In `@CefSharp.BrowserSubprocess.Core/JavascriptBindingSettings.h`:
- Around line 21-25: The subprocess constructor JavascriptBindingSettings()
currently sets JavascriptBindingApiEnabled = true which contradicts the
documented default (false) and causes unintended injection; change the
constructor to set JavascriptBindingApiEnabled = false (keep the existing
initializations for JavascriptBindingApiHasAllowOrigins and
JavascriptBindingApiAllowOrigins intact) so the subprocess default matches
CefSharp/JavascriptBinding/JavascriptBindingSettings.cs and
IsJavascriptBindingApiAllowed behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bfe720d1-0267-4703-bec5-1c4c33f091c1
📒 Files selected for processing (6)
CefSharp.BrowserSubprocess.Core/CefAppUnmanagedWrapper.cppCefSharp.BrowserSubprocess.Core/CefAppUnmanagedWrapper.hCefSharp.BrowserSubprocess.Core/CefBrowserWrapper.hCefSharp.BrowserSubprocess.Core/CefSharp.BrowserSubprocess.Core.netcore.vcxprojCefSharp.BrowserSubprocess.Core/CefSharp.BrowserSubprocess.Core.vcxprojCefSharp.BrowserSubprocess.Core/JavascriptBindingSettings.h
💤 Files with no reviewable changes (1)
- CefSharp.BrowserSubprocess.Core/CefBrowserWrapper.h
✅ Files skipped from review due to trivial changes (2)
- CefSharp.BrowserSubprocess.Core/CefSharp.BrowserSubprocess.Core.vcxproj
- CefSharp.BrowserSubprocess.Core/CefSharp.BrowserSubprocess.Core.netcore.vcxproj
|
✅ Build CefSharp 146.0.70-CI5457 completed (commit 02011e55d0 by @campersau) |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
CefSharp.BrowserSubprocess.Core/CefAppUnmanagedWrapper.h (1)
30-54:⚠️ Potential issue | 🔴 CriticalAdd per-browser cleanup for
_browserJavascriptBindingSettingsinOnBrowserDestroyed.
_browserJavascriptBindingSettingsentries are added when browsers are created but never removed when destroyed. Currently, cleanup only occurs at process teardown (destructor). In long-lived subprocesses with multiple browser creations/destructions, this causes unbounded accumulation of stale per-browser settings. Add aTryRemovecall inOnBrowserDestroyedto clean up the entry for the destroyed browser ID, matching the pattern already used for_browserWrappers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CefSharp.BrowserSubprocess.Core/CefAppUnmanagedWrapper.h` around lines 30 - 54, OnBrowserDestroyed currently removes entries from _browserWrappers but does not remove the per-browser settings in _browserJavascriptBindingSettings, causing leaks; modify OnBrowserDestroyed to call TryRemove on _browserJavascriptBindingSettings using the same browser id (the key used with _browserWrappers) to remove the destroyed browser's JavascriptBindingSettings entry (mirror the existing removal pattern for _browserWrappers and ensure any associated resources are released).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@CefSharp.BrowserSubprocess.Core/CefAppUnmanagedWrapper.h`:
- Around line 30-54: OnBrowserDestroyed currently removes entries from
_browserWrappers but does not remove the per-browser settings in
_browserJavascriptBindingSettings, causing leaks; modify OnBrowserDestroyed to
call TryRemove on _browserJavascriptBindingSettings using the same browser id
(the key used with _browserWrappers) to remove the destroyed browser's
JavascriptBindingSettings entry (mirror the existing removal pattern for
_browserWrappers and ensure any associated resources are released).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a472c299-1812-48fa-b0e4-3ae109c6e717
📒 Files selected for processing (1)
CefSharp.BrowserSubprocess.Core/CefAppUnmanagedWrapper.h
|
✅ Build CefSharp 146.0.70-CI5458 completed (commit dc7a0db18f by @campersau) |
|
Thanks for the quick response, greatly appreciated. There are some follow up changes like moving the js property names to the new object. Could possibly change TryAdd to GetOrAdd, or skipping processing of the properties if the TryAdd returns false. I don't think these changes are blocking, so let's progress this as is. Will merge we I'm in front on a computer. |
* Allow JavaScript binding when browser wrapper cannot be found * Store JavascriptBindingSettings per Browser and never remove them This avoids the issue that sometimes browsers are destroyed but then referenced again without them beeing created * Use dict->Values
4 participants
Fixes: #5228
Summary: Fixes regression to allow JavaScript bindings when browser wrapper can not be found.
Changes:
How Has This Been Tested?
Manually.
My App works again with this build: https://ci.appveyor.com/project/campersau/cefsharp/builds/53796822/artifacts
Screenshots (if appropriate):
Types of changes
Checklist:
Summary by CodeRabbit